Refactor TMTTableFlusher #2
Conversation
flowbehappy
force-pushed
the
region-flush-refactor
branch
from
242f51e to
46b8d25
Compare
| @@ -94,7 +94,7 @@ void dbgFuncDropTiDBTable(Context & context, const ASTs & args, DBGInvoker::Prin | |||
|
|
|||
| TMTContext & tmt = context.getTMTContext(); | |||
| tmt.region_partition.dropRegionsInTable(table_id); | |||
| tmt.table_flushers.dropRegionsInTable(table_id); | |||
| tmt.region_partition.dropRegionsInTable(table_id); | |||
| { | ||
| auto & partition = getOrInsertRegion(table_id, region_id).second; | ||
| partition.updated = true; | ||
| partition.cache_bytes += delta; |
There was a problem hiding this comment.
Say for a specific region r1 containing two tables t1 and t2, a raft command put 10 bytes of t1 and 10 bytes of t2 into r1. Then delta here would be 20 bytes, and would be added to relative tables (both t1 and t2). That is not accurate. Am I correct?
There was a problem hiding this comment.
It is correct. But actually we don't need to be 100% percent accurate. And a region belonging to more than one table is very rare.
There was a problem hiding this comment.
How about let Region::onCommand return table id and delta bytes pair and pass into updateRegion? So that in updateRegion we could know how many bytes each table changes. This doesn't require much code I assume, but I could be wrong.
There was a problem hiding this comment.
We can theoretically, but I still don't think it is necessary to make the logic more complicated. It doesn't loss anything even if we flush a partition by mistake. And again this situation is very rare.
There was a problem hiding this comment.
I just thought this wouldn't take much code, and the logic could be quite less confusing, so worth doing. But it's your call after all.
There was a problem hiding this comment.
Maybe I should add some documents to help reducing confuses.
| // drop table and create another with same name, but the previous one will still flush | ||
| if (storage == nullptr) | ||
| { | ||
| LOG_ERROR(log, "table " << table_id << " flush partition " << partition_id << " , but storage is not found"); |
There was a problem hiding this comment.
In what situation will the storage be null? Does it imply some potential risks that we should do more than simply log and ignore (return), like throwing an exception?
There was a problem hiding this comment.
Indeed we should throw an exception. Some potential bugs may cause this I guess. Although haven't found it happen yet.
| } | ||
| } | ||
|
|
||
| void RegionPartition::updateRegion(const RegionPtr & region, size_t before_cache_bytes, TableIDSet relative_table_ids) |
There was a problem hiding this comment.
I saw updateRegion was after Region::onCommand. What if background flush happens in between? More specifically: 1. some data was inserted into a region; 2. background flush grabbed the mutex in RegionPartition and flushed the data of the region (time limit reached for example); 3. updateRegion then executed, it saw an empty region with wrong update arguments (before_cache_bytes for example).
There was a problem hiding this comment.
The risk does exist. And it is one of the many multi-thread issues in our system. I would like to file an issue and fix it until we have a better global design. https://internal.pingcap.net/jira/browse/FLASH-132
|
LGTM as long as comments addressed. |
| TableID end_table_id = end_key.empty() ? std::numeric_limits<TableID>::max() : RecordKVFormat::getTableId(end_key); | ||
|
|
||
| // This region definitely does not contain any data in this table. | ||
| if (start_table_id > table_id || end_table_id < table_id) |
There was a problem hiding this comment.
flowbehappy
force-pushed
the
region-flush-refactor
branch
from
109554e to
fb67fcd
Compare
This PR asynchronize the flush process from raft log apply process.